Support memoryview encoding/decoding as a no-op, calling tobytes() on… #1285

Cody-G · 2020-02-08T21:23:23Z

…ly when decode_responses=True

Pull Request check-list

Please make sure to review and check all of these items:

Does $ tox pass with this change (including linting)?
Does travis tests pass with this change (enable it first in your forked repo and wait for the travis build to finish)?
Is the new or changed code fully tested?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Description of change

Supports passing memoryviews to redis-py as first outlined in #1265 and discussed further in #1266

…ly when decode_responses=True

…mmand()

codecov-io · 2020-02-09T00:15:57Z

Codecov Report

Merging #1285 into master will increase coverage by 0.26%.
The diff coverage is 97.91%.

@@            Coverage Diff             @@
##           master    #1285      +/-   ##
==========================================
+ Coverage   92.45%   92.72%   +0.26%     
==========================================
  Files          19       19              
  Lines        6415     6444      +29     
==========================================
+ Hits         5931     5975      +44     
+ Misses        484      469      -15

Impacted Files	Coverage Δ
tests/test_encoding.py	`97.59% <100%> (+1.59%)`	⬆️
tests/test_pipeline.py	`100% <100%> (ø)`	⬆️
redis/connection.py	`82.02% <90%> (+1.31%)`	⬆️
tests/conftest.py	`97.53% <0%> (-2.47%)`	⬇️
tests/test_monitor.py	`100% <0%> (ø)`	⬆️
tests/test_commands.py	`99.94% <0%> (ø)`	⬆️
redis/client.py	`86.09% <0%> (+0.28%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d8d523...5750b4c. Read the comment docs.

redis/connection.py

andymccurdy

Some suggestions and questions inline below. And two suggestions overall:

It would be nice to add a simple memoryview test going through a pipeline. The changes to Connection.pack_commands aren't tested otherwise as Pipeline is the only thing calling pack_commands().

Further, I'd propose that we subclass memoryview and override its tobytes() to emit a warning. Then we could use that subclass in the test suite instead of the base memoryview. That should hopefully confirm that these optimizations are actually useful.

redis/connection.py

tests/test_encoding.py

andymccurdy · 2020-02-12T22:24:32Z

redis/connection.py

-            value = value.decode(self.encoding, self.encoding_errors)
+        "Return a unicode string from the bytes-like representation"
+        if (self.decode_responses or force):
+            if isinstance(value, memoryview):


Did you run into a case where you needed this? Are we ever actually decoding memoryview instances?

I don't think I ran into a case in the tests where a memoryview gets decoded. I think I got a bit spooked by lines like this and this, but on a closer look those lines shouldn't cause a problem unless the user passes a memoryview kwarg -- and the docs instruct them to pass strings. Maybe it just felt right to make sure that decode(encode(x)) always works, but it seems it's not essential, happy to remove this if you prefer.

Cody-G · 2020-02-16T02:15:23Z

Thanks for the review! I think most of the changes are straightforward. I like the idea of subclassing memoryview, but I found out that's not possible. Quoting from the docs: "It is not currently allowed to create subclasses of memoryview." So I think we can't use a custom class to test that we're never calling tobytes(), since the code relies on isinstance(x, memoryview). What we really care about is whether the argument is still a memoryview by the time it gets sent over the socket by sendall here, so testing the return value of pack_command() should be sufficient (it gets called here).

Edit: and also testing pack_commands by using a pipeline as was suggested in the review.

Cody-G · 2020-02-16T02:51:35Z

This seems ready for another review. To summarize the changes, everything was straightforward except:

I couldn't subclass memoryview to make a warning-emitting view. But I think the tests to pack_command and pack_commands are sufficient to test that tobytes() doesn't get called (i.e. that we're saving a memory copy)
It's undecided whether we should allow Encoder.decode to work with memoryviews (see my comment for details).

I also noticed that the argument length-checking and joining code in pack_commands is redundant with that of pack_command. pack_commands always calls pack_command and they process arguments in the same way. It's probably safe to remove the checking from pack_commands, but I haven't done so in this PR.

andymccurdy · 2020-02-24T23:40:48Z

Thanks @Cody-G. This has been merged.

Cody-G mentioned this pull request Feb 8, 2020

Support memoryview encoding (as a no-op) #1266

Closed

3 tasks

Cody-G added 3 commits February 8, 2020 14:35

Support memoryview encoding/decoding as a no-op, calling tobytes() on…

cdae13d

…ly when decode_responses=True

Test that memoryview arguments don't get packed by Connection.pack_co…

d23273b

…mmand()

fix tox formatting errors

4f0eed6

jdufresne reviewed Feb 12, 2020

View reviewed changes

redis/connection.py Outdated Show resolved Hide resolved

redis/connection.py Outdated Show resolved Hide resolved

andymccurdy suggested changes Feb 12, 2020

View reviewed changes

Cody-G added 2 commits February 15, 2020 17:29

clean up if statement formatting

6239504

Split encoding and encoding+decoding into separate tests

58eb05e

Cody-G added 2 commits February 15, 2020 18:27

ensure that pack_commands does not copy memoryviews

7946f73

Test that pipelines work with memoryviews

5750b4c

Cody-G requested a review from andymccurdy February 16, 2020 02:44

andymccurdy closed this in 3310fe4 Feb 24, 2020

Cody-G deleted the cjg/memoryviews branch February 25, 2020 19:58

dependabot bot mentioned this pull request Mar 17, 2021

Bump redis from 3.4.1 to 3.5.3 in /requirements tchellomello/awx#8

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support memoryview encoding/decoding as a no-op, calling tobytes() on… #1285

Support memoryview encoding/decoding as a no-op, calling tobytes() on… #1285

Uh oh!

Cody-G commented Feb 8, 2020 •

edited

Loading

Uh oh!

codecov-io commented Feb 9, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

andymccurdy left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andymccurdy Feb 12, 2020

Uh oh!

Cody-G Feb 16, 2020

Uh oh!

Cody-G commented Feb 16, 2020 •

edited

Loading

Uh oh!

Cody-G commented Feb 16, 2020 •

edited

Loading

Uh oh!

andymccurdy commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Support memoryview encoding/decoding as a no-op, calling tobytes() on… #1285

Support memoryview encoding/decoding as a no-op, calling tobytes() on… #1285

Uh oh!

Conversation

Cody-G commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request check-list

Description of change

Uh oh!

codecov-io commented Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

andymccurdy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andymccurdy Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

Cody-G Feb 16, 2020

Choose a reason for hiding this comment

Uh oh!

Cody-G commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cody-G commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andymccurdy commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Cody-G commented Feb 8, 2020 •

edited

Loading

codecov-io commented Feb 9, 2020 •

edited

Loading

Cody-G commented Feb 16, 2020 •

edited

Loading

Cody-G commented Feb 16, 2020 •

edited

Loading